Skip to content

fix(as_llm): close owned OpenAI clients safely - #725

Open
fredheathcote007 wants to merge 3 commits into
agentscope-ai:mainfrom
fredheathcote007:upstream/fix-openai-client-lifecycle-20261107
Open

fredheathcote007 wants to merge 3 commits into
agentscope-ai:mainfrom
fredheathcote007:upstream/fix-openai-client-lifecycle-20261107

Conversation

@fredheathcote007

Copy link
Copy Markdown

Summary

  • Close internally owned asynchronous OpenAI chat clients when their ReMe component is closed; retain externally injected clients as borrowed resources.
  • Finish provider cleanup safely under repeated cancellation and handle failed close/startup without losing ownership tracking.
  • Account for lazy client construction before startup, model replacement, shutdown/restart and access after shutdown.
  • Add adversarial lifecycle tests for these boundaries while preserving existing component behaviour.

Validation

  • Python 3.12: 42 passed across OpenAI model-adapter and base-component lifecycle tests.
  • Python compilation, focused Ruff correctness checks and git diff --check passed.

Scope

Only the OpenAI chat adapter and its unit tests are changed. No dependencies or configuration defaults are changed.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ployts ployts left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core fix is worthwhile: internally created OpenAI clients need explicit cleanup, while injected models should remain borrowed. The normal sequential lifecycle paths look correct. I found two reproducible concurrency gaps in the added lifecycle handling that I suggest addressing before merging; details are inline.

Validation: 42 focused tests in test_as_llm_lazy.py and test_base_component.py passed in the available Python 3.12 environment. Both findings were reproduced with mocked clients. The full suite, pre-commit, and provider integration tests were not run.

return
await super().close()
finally:
self._closed = True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Prevent lazy model construction throughout shutdown

_release_model() clears _model in its separate task before the outer close() resumes and sets _closed here. A concurrent llm.model access in that window constructs a second owned client. Shutdown then returns with is_started=False, _closed=True, and _owns_model=True, and the second client's close() was never awaited. The getter also keeps returning that model after shutdown because it checks _model before _closed.

I reproduced this by setting an asyncio.Event in the first client's async close method and having another task wait for that event before reading llm.model. Please make the transition into shutdown prevent new model construction before provider cleanup can yield, coordinate that transition with the lifecycle lock, and add a regression test for this interleaving.

except BaseException:
if self._owns_model and self._model is not None:
try:
await self._close()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not clean up another lifecycle operation's model when a startup waiter is cancelled

super().start() can raise CancelledError while this invocation is still waiting to acquire _lock. This handler then calls _close() without owning that lock, even though another startup or shutdown operation owns the model. With a running component whose client close is blocked, queueing a start() and cancelling it causes client.close() to run twice concurrently. With one startup blocked and a pre-created model, cancelling a second queued startup closes and discards the first startup's model while that startup is still running.

Please distinguish cancellation before acquiring the lifecycle lock from failure within this startup invocation, and serialize rollback with the other lifecycle operations. A cancelled lock waiter should not tear down their resources.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants